351 add relationship info #505
Conversation
… 351-add-relationship-info
leekahung
added
enhancement
|
Hey @ofu997! Thanks for your contribution to the project. I have a general sense of what the changes are, but would like to have the PR description updated to detail what you've done/changed for this PR. It'll help let other users get a feel for what this PR is for. Also, keep note of unused imports and other errors provided by the linter and update the codebase accordingly (see lint check on GitHub: https://github.com/codeforpdx/PASS/actions/runs/6794649087/job/18471386525?pr=505). |
There was a problem hiding this comment.
Hey @ofu997! Just gone through some of the code. It looks like this aims to add new fields to the contacts object that we'll be storing with the contactsList, which I think is good. However, I'm not too sure about adding them as adjustable fields in src/components/Modals/AddContactModal.jsx just yet. Think we'll need more discussion about how to implement these new properties.
There are also other thing that need addressing like the unit tests. See feedback below.
… 351-add-relationship-info
|
Also, there seems to be linting errors. Do you run the lint or prettier in your branch? It doesn't seem like prettier is being picked up in your PR. |
|
I ran ```npm run prettier:test``` without error.
…On Wed, Nov 15, 2023 at 12:17 PM Ka Hung Lee ***@***.***> wrote:
Also, there seems to be linting errors. Do you run the lint or prettier in
your branch? It doesn't seem like prettier is being picked up in your PR.
—
Reply to this email directly, view it on GitHub
<#505 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFPDS72QAMM3HN3U373PL5TYEUPMXAVCNFSM6AAAAAA7CLCSMCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJTGE4TSNZQHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
We don't have a script for 
|
|
Weird |
You need to run Note the error found in the check:
|
|
Hey @ofu997 ! Wanted to check on the progress of this PR. Believe we could start getting the discussions for where to place these new fields with the UX team once we got this merged and start an issue opened for implementation. |
… 351-add-relationship-info
There was a problem hiding this comment.
The structure of the "role" property seems incorrect. We're storing a string to it, but the definition on schema.org describes an object (technically a "thing") with a few different attributes.
The documentation actually links to a blog post about it: http://blog.schema.org/2014/06/introducing-role.html
… 351-add-relationship-info
|
I'll make it https://schema.org/roleName |
That looks like a good solution to me |
|
Hey @ofu997! Was wondering if this is still being worked on. If you are, let us know. Thanks! |
Hi Kahung, |
…rpdx/PASS into 351-add-relationship-info
This PR:
Resolves #351
Screenshots (if applicable):
Additional Context (optional):
Add relation and relationship status to contact. We are currently (Nov 14) not sure whether we will set this in the add contact modal or from the table in the contact list.
Currently this creates two new constants files, and changes the useContactsList hook.
Future Steps/PRs Needed to Finish This Work (optional):
Add any other steps/PRs that may be needed to continue this work if this PR is just a step in the right direction.
Issues needing discussion/feedback (optional):
1. I'm unable to do the pod field which is the second field in the screenshot.
add this field: pod: <podUrl>, // optional: the person's preferred pod url. If not present, we fetch the first pod url on the profile document. If that's not present, we derive it from the webid.When I tried to do it in previous commits there would be Promise or auth errors. This needs to be done later as a separate issue.